operator: use infra and network manifests to create controllerconfigspec #357
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
approved
|
working on this, this is not ready for review. using it as a way to get release for local testing. |
|
also needs #351 to sync the |
abhinavdahiya
force-pushed
the
move_to_global_config
branch
5 times, most recently
from
f052bdd
to
4e00452
Compare
|
Is this 4.0 work? |
abhinavdahiya
force-pushed
the
move_to_global_config
branch
3 times, most recently
from
fa9a32b
to
0a261d8
Compare
|
sweet, this is green :yay: i'll start to cleanup and add details. |
|
It looks like it is based on the referenced PR. @abhinavdahiya can you confirm? |
Yes i need this PR to make progress on openshift/installer#1169. I will be filing for exception for all of it together. |
|
/hold Since the CI is green, holding until the exception is granted. |
openshift-ci-robot
added
the
do-not-merge/hold
abhinavdahiya
force-pushed
the
move_to_global_config
branch
from
0a261d8
to
fb20302
Compare
abhinavdahiya
changed the title
openshift-ci-robot
added
needs-rebase
Since [1], the `Infrastructure.config.openshift.io` global config object contains the kube apiserver's url and the domain that needs to be used by etcd to do bootstrapping using dns discovery. The installer creates and sets the appropiate value since [2]. Therefore there is no longer a requirement for cluster_name and base_domain to create these values in template controller. [1]: openshift/api#189 [2]: openshift/installer#1155
abhinavdahiya
force-pushed
the
move_to_global_config
branch
from
fb20302
to
87b6b26
Compare
openshift-ci-robot
removed
the
needs-rebase
This is required for openshift/installer#1169 and openshift/installer#1169 has approval. So ping @runcom @cgwalters |
infra object provides the etcd_discover_domain [1] and api_server_url, while the network object provides the service_network [3] used to calculate the cluster_dns_ip. currently the network objects's `ServiceNetwork` list only suports single service network [3] and therefore it is safe to only use the first element. [1]: https://github.com/openshift/api/blob/4912e102a00fa8bb4bf27f83808dce6ef1b6887d/config/v1/types_infrastructure.go#L42 [2]: https://github.com/openshift/api/blob/4912e102a00fa8bb4bf27f83808dce6ef1b6887d/config/v1/types_infrastructure.go#L47 [3]: https://github.com/openshift/api/blob/4912e102a00fa8bb4bf27f83808dce6ef1b6887d/config/v1/types_network.go#L32
abhinavdahiya
force-pushed
the
move_to_global_config
branch
from
87b6b26
to
936edd2
Compare
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
Gave this code a slightly-more-than-superficial review; overall LGTM. |
| @@ -51,16 +51,6 @@ type MCOConfig struct { | |||
|
|
|||
| // MCOConfigSpec is the spec for MCOConfig resource. | |||
| type MCOConfigSpec struct { | |||
There was a problem hiding this comment.
Is this just an empty struct?
There was a problem hiding this comment.
Yes, we don't need to duplicate here and controllerconfigspec.
There was a problem hiding this comment.
The one place it was used was deleted too (operator/render.go) so can I ask to understand why we keep it?
There was a problem hiding this comment.
k8s object typically have spec and status. So this needs to staty but, for now empty.
MCOConfig can be expanded later on to drive operator level configuration like
maybe placement of controller, server etc.
log levels for its operands and such...
There was a problem hiding this comment.
(Abhinav was faster than me at commenting so I didn't notice his comment before posting mine)
My take is this is still the MCO config object and we may add fields going forward (as well as deprecating and so on and so forth) so we stick it around but if we instead can remove it, let's nuke it
There was a problem hiding this comment.
makes sense! @abhinavdahiya and @runcom (tag team!)
|
Apart from @kikisdeliveryservice comments, this LGTM, I'll properly lgtm it once comments are addressed |
|
Thanks for the updates and answers @abhinavdahiya . Was very helpful! 😺 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Bug 1930570: Update Jenkins monitored templates names
cluster-config-v1is going to be deprecated in favor of global configs 1. So moving the 2 major options that MCO requires etcd discovery domain and apiserver url toInfrastructure.config.openshift.io2. As of 3 the installer is now setting those values and MCO can use global configs to fetch those 2 specific information.